VPC: associate tier public IPs when a public IP of the tier has firewall rules by adestis-mf · Pull Request #14681 · apache/cloudstack

adestis-mf · 2026-11-11T17:06:53Z

Description

This PR fixes adding and removing public IPs of a VPC tier on the VPC virtual router when the VPC
offering provides the Firewall service for public IPs (#12706).

IpAddressManagerImpl.applyIpAssociations passes VpcVirtualRouterElement.applyIps the union of
the services of the public IPs of a tier. Once a public IP of the tier has a firewall rule,
Firewall is part of that union. applyIps associates the IPs only when
canHandle(tier, service) is true for every service, and the tier's network offering does not
list the Firewall service, because #12706 puts it on the VPC offering. applyIps then skips the
association and returns true:

a new public IP with port forwarding or load balancing rules in that tier never reaches the
router, also not on a network restart;
a public IP whose last rule is removed stays configured on the router.

applyIps now checks the Firewall service on the VPC when the tier does not provide it
(canHandle(Vpc, Firewall), the same check as in applyFWRulesInVPC). Other services and
canHandle(Network, Service) are unchanged, so testCanHandleFirewallUsesVpcCapability still
holds. A debug line is logged when the association is skipped, so the case is no longer silent.

What changes: in a VPC whose offering provides the Firewall service through the VPC virtual
router, public IPs of a tier are added to and removed from the router the same way as in a VPC
without the Firewall service. The IPs passed to the element are the same as before. IPs that have
only firewall rules and the source NAT IP of the VPC are not associated with a tier and are still
not passed. VPC offerings whose Firewall provider is not the VPC virtual router behave as before.

This targets main: #12706 is not in 4.22, and there is no 4.23 branch. The commit also
applies cleanly on top of the 4.23.0.0 tag.

Fixes: #14146
Fixes: #14669

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
Build/CI
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

New unit tests in VpcVirtualRouterElementTest:

testApplyIpsAssociatesTierIpsWhenTheVpcProvidesFirewall: the tier does not provide Firewall,
the VPC does → the IPs are associated.
testApplyIpsSkipsAssociationWhenNeitherTierNorVpcProvidesFirewall: neither provides it → no
association, no router lookup.
testCanHandleIpServiceUsesTheVpcOnlyForFirewall: other services never consult the VPC.
testCanHandleIpServiceKeepsTierFirewall: when the tier provides Firewall, the VPC is not
consulted.

With the previous check (canHandle(network, service)) put back into applyIps,
testApplyIpsAssociatesTierIpsWhenTheVpcProvidesFirewall fails.

mvn -pl server -am test -Dtest=VpcVirtualRouterElementTest \
    -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false

VpcVirtualRouterElementTest: 8 tests, 0 failures. The tests in com.cloud.network.element,
com.cloud.network.router, com.cloud.network.vpc, com.cloud.network.lb,
com.cloud.network.firewall, IpAddressManager*Test and NetworkModel*Test also pass (352
tests, 0 failures, 4 skipped). Checkstyle reports no violations.

The behaviour without this change was reproduced on 4.23.0.0; steps and results are in #14146.
The patched build has not been run on a live system.

How did you try to break this feature and the system with this change?

I went through the cases in which the changed check decides differently:

public IPs with only firewall rules and the VPC source NAT IP are not associated with a tier,
so they are not passed to applyIps, as before;
static NAT passes only its own IPs; the change only matters there when the static NAT IP itself
has a firewall rule;
redundant VPC routers are handled by the same loop over the routers of the tier as without
firewall rules;
other services, and VPC offerings with another Firewall provider, keep the previous result.

…all rules Since apache#12706 the Firewall service for public IPs of a VPC is provided by the VPC offering; the network offerings of the tiers do not list it. applyIpAssociations passes the services of the public IPs of a tier to VpcVirtualRouterElement.applyIps, which associates the IPs only when it can handle every one of these services on the tier. Once any public IP of the tier has a firewall rule, Firewall is in that set, canHandle(tier, Firewall) is false, and applyIps skips the association without an error: - port forwarding and load balancing rules on a new IP are applied, but the IP never reaches the router, also not on a network restart; - an IP whose last rule is removed stays configured on the router. Check the Firewall service on the VPC in that case, as applyFWRulesInVPC does. Other services and canHandle(network, service) are unchanged. Log when the IP association is skipped.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

2 open issues

_vpcDao.findById(network.getVpcId()) can return null; passing that into canHandle(...) risks a… · New
These helpers repeat hard-coded IDs (1L, 100L, 200L) across multiple stubbings. Consider… · New

What changed in this PR

This PR fixes VPC tier public IP association logic so tiers can still associate IPs on the VPC virtual router when the Firewall service is provided by the VPC offering (not the tier network offering), and adds unit tests to cover the corrected behavior.

Changes:

Update applyIps to use a new canHandleIpService check that consults VPC capability specifically for the Firewall service.
Add debug logging when IP association is skipped due to service handling constraints.
Add unit tests validating the new association behavior and ensuring only Firewall consults the VPC.

File	Description
server/src/main/java/com/cloud/network/element/VpcVirtualRouterElement.java	Adds `canHandleIpService` and updates `applyIps` to handle Firewall via VPC capability checks, plus debug logging when skipping association.
server/src/test/java/com/cloud/network/element/VpcVirtualRouterElementTest.java	Adds focused unit tests for `applyIps` behavior when Firewall is (or isn’t) provided by the VPC, and tests for `canHandleIpService` decision boundaries.

_{Review effort: Lite}

adestis-mf · 2026-11-11T17:43:04Z

+            return true;
+        }
+        if (Service.Firewall.equals(service) && network.getVpcId() != null) {
+            return canHandle(_vpcDao.findById(network.getVpcId()), service);


canHandle(Vpc, Service) already returns false for a null VPC (it is the first check in that
method), so there is no NullPointerException here: if the VPC cannot be found, the IP association is
skipped, as it was before this change, and the new debug line logs that. applyFWRulesInVPC goes
through the same method. I would leave it as it is.

adestis-mf · 2026-11-11T17:43:12Z

+    private Network mockVpcTier() {
+        final Network network = Mockito.mock(Network.class);
+        when(_networkMdl.getPhysicalNetworkId(network)).thenReturn(1L);
+        when(network.getId()).thenReturn(200L);
+        when(network.getVpcId()).thenReturn(100L);
+        when(_networkMdl.isProviderEnabledInPhysicalNetwork(1L, Network.Provider.VPCVirtualRouter.getName())).thenReturn(true);
+        return network;
+    }
+
+    private VpcVO mockVpc(final boolean vpcProvidesFirewall) {
+        final VpcVO vpc = Mockito.mock(VpcVO.class);
+        when(vpc.getId()).thenReturn(100L);
+        when(vpc.getZoneId()).thenReturn(1L);
+        when(_vpcDao.findById(100L)).thenReturn(vpc);
+        when(_networkMdl.isProviderEnabledInZone(1L, Network.Provider.VPCVirtualRouter.getName())).thenReturn(true);
+        when(_vpcMgr.isProviderSupportServiceInVpc(100L, Network.Service.Firewall, Network.Provider.VPCVirtualRouter)).thenReturn(vpcProvidesFirewall);
+        return vpc;
+    }


The new helpers use the same values as the existing testCanHandleFirewallUsesVpcCapability in this
class, which has 1L, 100L and 200L inline as well. I kept that style so the new tests read like
the existing one and the change stays small. Happy to switch both to named constants if reviewers
prefer.

adestis-mf · 2026-11-11T17:51:20Z

@RosiKyu @nvazquez could this be considered for the 24.0 milestone? It fixes #14146 (moved to 25.0 earlier today) and #14669: in a VPC whose offering includes the Firewall service (#12706, new in 4.23.0.0), once a public IP of a tier has a firewall rule, further public IPs of that tier are no longer added to or removed from the virtual router. The change is limited to one method in VpcVirtualRouterElement and covered by unit tests. It also matters for the CKS fix in #14670, which adds firewall rules on the cluster IP.

RosiKyu · 2026-11-11T18:53:25Z

@adestis-mf , sure. Thank you for the clarification. Have mmoved #14146 back to 24.0

RosiKyu · 2026-11-11T20:34:07Z

@blueorangutan package

blueorangutan · 2026-11-11T20:36:06Z

@RosiKyu a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-11-11T22:18:39Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 20004

Copilot AI balanced review requested due to automatic review settings November 11, 2026 17:06

boring-cyborg Bot added the component:networking label Nov 11, 2026

Copilot AI reviewed Nov 11, 2026

View reviewed changes

Copilot stopped reviewing on behalf of adestis-mf due to an error November 11, 2026 17:53
The job was not acquired by a runner even after multiple attempts.

RosiKyu requested review from DaanHoogland, RosiKyu, nvazquez and weizhouapache November 11, 2026 18:59

RosiKyu added this to the 24.0 milestone Nov 11, 2026

Navigation Menu

Repository navigation

VPC: associate tier public IPs when a public IP of the tier has firewall rules - #14681

VPC: associate tier public IPs when a public IP of the tier has firewall rules#14681
adestis-mf wants to merge 1 commit into
apache:mainfrom
adestis-mf:vpc-firewall-ip-association

adestis-mf commented Nov 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

adestis-mf Nov 11, 2026

Uh oh!

adestis-mf Nov 11, 2026

Uh oh!

adestis-mf commented Nov 11, 2026

Uh oh!

RosiKyu commented Nov 11, 2026

Uh oh!

RosiKyu commented Nov 11, 2026

Uh oh!

blueorangutan commented Nov 11, 2026

Uh oh!

blueorangutan commented Nov 11, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

adestis-mf commented Nov 11, 2026

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

adestis-mf Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

adestis-mf Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

adestis-mf commented Nov 11, 2026

Uh oh!

RosiKyu commented Nov 11, 2026

Uh oh!

RosiKyu commented Nov 11, 2026

Uh oh!

blueorangutan commented Nov 11, 2026

Uh oh!

blueorangutan commented Nov 11, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants